Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add Context Menu to Runs view (ET-232) #9480

Merged
merged 172 commits into from
Jul 2, 2024
Merged

feat: add Context Menu to Runs view (ET-232) #9480

merged 172 commits into from
Jul 2, 2024

Conversation

johnkim-det
Copy link
Contributor

@johnkim-det johnkim-det commented Jun 6, 2024

Ticket

ET-232

Description

Adds context menu (menu which appears on right-click of DataGrid cell) to Runs view.

This PR includes these actions in the menu:

  • Copy Value
  • Open in New Tab
  • Open in New Window
  • Archive
  • Unarchive
  • Delete
  • Kill
  • Move

This PR has changes from #9368 merged in. Planning not to merge this PR until that one is merged first. Should not actually include backend changes at that time. Code coverage numbers should hopefully also improve when the patch is only for context menu changes.

Most likely easier to review by looking at the diff between those two branches here: feature/actions-flat-run-table...ET-232

Test Plan

  • Cells with a value should show a functional "Copy Value" option
  • Cells with a link should show functional "Open in New Tab" and "Open in New Window" options

With permissions to modify, move, and delete experiments/runs:

  • Cells should have functional "Move"/"Delete" options
  • Cells for a run with killable state (for example, "Active" or "Paused") should show a functional "Kill" option
  • Cells for an unarchived run should show functional "Archive" option
  • Cells for an archived run should show functional "Unarchive" option

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@johnkim-det johnkim-det removed the request for review from a team June 28, 2024 15:19
@keita-determined
Copy link
Contributor

can we please add more test cases? the coverage is 67.66055%

@johnkim-det
Copy link
Contributor Author

@keita-determined updated w more tests

Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added few comments, otherwise \o/

Comment on lines 170 to 175
if (cell) {
if ('displayData' in cell && isString(cell.displayData)) {
return cell.displayData;
}
if (cell.copyData) return cell.copyData;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can simplify the code for the readability, but up to you

Suggested change
if (cell) {
if ('displayData' in cell && isString(cell.displayData)) {
return cell.displayData;
}
if (cell.copyData) return cell.copyData;
}
if (cell && 'displayData' in cell && isString(cell.displayData)) {
return cell.displayData;
}
if (cell && cell.copyData) return cell.copyData;

Comment on lines 65 to 71
if (cell) {
if ('displayData' in cell && isString(cell.displayData)) {
return cell.displayData;
}
if (cell.copyData) return cell.copyData;
}
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we fix the following warning?

Warning: An update to ForwardRef inside a test was not wrapped in act(...).

When testing, code that causes React state updates should be wrapped into act(...):

act(() => {
  /* fire events that update state */
});
/* assert on the output */

@johnkim-det johnkim-det merged commit ebf2398 into main Jul 2, 2024
81 of 94 checks passed
@johnkim-det johnkim-det deleted the ET-232 branch July 2, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed feature-flagged The changes in this PR are behind a feature flag
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants